🤖 AI Code Review (two independent reviewers)

Reviewer responses: codex/ALL claude/ALL

Status: BLOCKED — CRITICAL/MAJOR issues must be resolved before merge

All verification is complete. Final synthesized chair review:

Chair Synthesis — PR #257: feat(inventory): collect ENIs, RDS clusters and RDS Proxies for topology discovery

1. Summary

The PR adds three read-only Steampipe inventory types (eni, rds_cluster, rds_proxy), a db_cluster_identifier column on rds, the matching 46→49 catalog-count updates across the release-gate docs, a new pytest contract file, and an ENI sizing runbook note. The collector-side mechanics are correct — I verified the pinned-list ordering against the source-AST lockstep test (runtime-release.test.mjs:437, new QUERIES entries appended last, inserted before the 5 SDK types; 46+3=49) and confirmed no IAM widening is needed. However, the three new types flow unregistered into the fleet-overview aggregation (verified against base web/ code), and the ENI query's stated "no public-address data" contract is contradicted by the raw private_ip_addresses projection.

2. Issues per lens

L2 — Code correctness

MAJOR (claude flagged; codex missed; chair CONFIRMED against base code) — The three new types silently distort the fleet-overview KPIs. Chair-verified: web/app/api/inventory/summary/route.ts:100-115 aggregates every resource_type with fallback label: row.resource_type / group: 'Other'; web/lib/inventory-types.ts has no eni/rds_cluster/rds_proxy entry, and until this PR the UI registry covered all 46 collector types 1:1 (web/lib/inventory-types.test.ts:22,103 pin 46) — this PR breaks that lockstep for the first time with no exclusion, registry entry, or doc note. Concrete trigger: after the first eni sync (the PR's own measurement: ~470–520 ENIs per account), web/app/page.tsx:250 (byType sorted desc, top-12 bar chart) puts a raw-labelled eni bar in slot 1 and evicts a real type; total (summary/route.ts:115, page.tsx:703) inflates by ~500/account with plumbing that re-counts existing resources; the category donut gains an Other slice; and web/app/api/inventory/trend/route.ts:116 sums the new inventory_snapshots series into the trend total, so Fargate ENI churn (which the PR's own runbook note calls out) injects noise into the 7-day net-change KPI. No test binds the collector catalog to the UI registry, so CI stays green. Fix: register the three types in INVENTORY_TYPES (updating the 46-count tests) or explicitly exclude topology-only types from summary/trend.

MINOR (claude only) — rds_cluster has no engine filter: the RDS DescribeDBClusters control plane also returns Neptune/DocumentDB clusters, and the repo separately collects neptune_cluster from aws_neptune_db_cluster, so an existing Neptune cluster is persisted under two types and double-counted.

MINOR (claude only) — eni is the first type whose row count scales with task churn rather than fleet size, and sync() writes one Aurora round trip per row inside the fixed 120 s reserve. Acceptable as documented in the runbook; a duration alarm would be better than "watch the logs".

MINOR (claude only) — eni omits owner_id; for requester-managed/cross-account ENIs the connection account_id is not the interface owner, weakening the stated "resolve IP to owning resource" goal.

Chair-verified non-issues: the rds column addition needs no fallback update (only iam_role has a HYDRATE_FALLBACK_SQL entry); the new pytest file follows the existing import sync_lambda convention and its string assertions hold against the new SQL.

L3 — Security / AWS mutation safety

No findings — both models agree; chair concurs. All three additions are plain read-only SELECTs with no {account_id}/{owner_ids} interpolation; no AWS mutating call, no gated flag change, no Terraform/IAM edit (existing ec2:Describe*/rds:Describe* under the region-pinned read condition already cover the new API calls — no widening made, none needed). ADR-005 boundary intact. Credential-bearing columns (auth, role_arn, master_user_name) are deliberately excluded and test-pinned. The agent SQL-reader view's key allowlist does not expose the new private-IP/endpoint/SG fields to models.

L4 — Observability / data-integration correctness

MAJOR (both models flagged the same fact — claude MAJOR, codex MINOR; chair adopts MAJOR) — The ENI query's documented data contract is false as shipped. The comment states "IPv6 and public (association_*) addresses are not collected," and test_eni_query_carries_ownership_fields enforces it only via assert "association_" not in sql, which checks top-level column names. But the query selects raw private_ip_addresses, whose elements (per the DescribeNetworkInterfaces shape passed through by the pinned plugin) each carry a nested Association object with public IP, public DNS and IP-owner data — so public-address data is persisted into inventory_resources.data, and the new test provides false assurance about the very contract it exists to pin. Independent convergence by both models on the nested-Association fact is a strong signal and matches the AWS API shape. No consumer reads it today and nothing credential-bearing leaks, but in this contract-strict repo a merge-time-false, test-pinned data-scope claim is a defect: either narrow the projection (e.g. primary/private IP fields only) or correct the comment and the test's rationale.

MINOR (codex flagged MAJOR; claude did not flag; chair downgrades to MINOR) — rds_proxy collects only the default proxy endpoint from aws_rds_db_proxy; custom proxy endpoints live in aws_rds_db_proxy_endpoint (DescribeDBProxyEndpoints), so applications dialing custom endpoints will stay unresolved. Factually correct, but this PR ships no resolution consumer at all — it is a coverage/scope limitation to document and follow up, not a behavioral or policy failure, so it does not meet the MAJOR bar.

MINOR (claude only) — The strict 49-type zero-unknown release gate now hard-couples web releases to three manually verified column lists with no mechanical plugin-contract check; a single wrong column name fails that type and blocks releases. The least certain name is tags on aws_rds_db_proxy (a ListTagsForResource hydrate, not a DescribeDBProxies field) — confirm against the deployed plugin image before merge.

MINOR (claude only) — The new data is collected but unconsumed (no topology/flow/security-findings reader), and rds_cluster's security-relevant fields (storage_encrypted, deletion_protection, IAM auth) feed no CIS-style check; fine for a staged PR but worth a follow-up ticket reference.

L5 — Docs / ADR consistency

No gate-level issues — both models agree the 46→49 rewrite is complete and correct (claude exhaustively grepped remaining 46 occurrences; all are unrelated). ADR-005 posture unchanged; no README/ADR contradiction.

MINOR (claude only) — The runbook documents only the eni type; rds_cluster, rds_proxy and the new rds.db_cluster_identifier column (and their deliberate secret-column exclusions) are recorded only in code comments and test assertions.

MINOR (claude only; interacts with the L2 MAJOR) — The collector catalog (49) and UI registry (46) have now diverged with nothing stating the superset is intentional; a future contributor can plausibly "fix" either number. Resolving the L2 MAJOR should also settle this.

MINOR (claude only, informational) — The regenerated AGENTS.md claude-md-sha headers cannot be recomputed in this review context; prose matches the CLAUDE.md edits and only the two changed files were regenerated.

3. Suggestions

1. Before merge: register eni/rds_cluster/rds_proxy in web/lib/inventory-types.ts (with icons/nav placement and updated 46-count tests) or add an explicit topology-only exclusion in the summary and trend routes — and add a test binding the collector catalog to the UI registry (or asserting the intentional superset).
2. Before merge: reconcile the ENI public-address contract — narrow private_ip_addresses to primary/private fields, or fix the comment and replace the "association_" not in sql assertion with one that reflects what is actually excluded.
3. Confirm tags on aws_rds_db_proxy against the deployed plugin image; a wrong column blocks every web release via the 49-type gate.
4. Add WHERE engine NOT IN ('neptune','docdb') (or equivalent) to rds_cluster to avoid double-counting with neptune_cluster, and consider adding owner_id to eni.
5. Follow-up ticket: aws_rds_db_proxy_endpoint custom-endpoint collection, the topology consumers that will read these types, and a sentence in the runbook covering rds_cluster/rds_proxy.

4. Verdict

Two MAJOR findings stand after chair verification against the base code: the unregistered types visibly distort the landing-page fleet KPIs the moment the first ENI sync completes (L2), and the ENI query's test-pinned data-scope contract is false as written (L4, flagged independently by both models). Both are cheaply fixable within this PR.

IMAGE_COVERAGE: NOT_REQUIRED

Triggered by commit 55f755b018840de2dd0e855b8187323d164ff80c · workflow: .github/workflows/pr-review.yml

Review response (commit 55f755b):

• MAJOR KPI leak: resolved by the rollout order. feat(topology): discover IP-target owners from ENIs and bind traces to Aurora/Proxy endpoints #258 now merges first: it registers eni/rds_cluster/rds_proxy in INVENTORY_TYPES and excludes ENIs from fleet totals, category counts and net-change parity. See the updated "Rollout order".
• MAJOR ENI write budget:
  • Measured about 470–520 ENIs per account in ap-northeast-2 and 41 in us-east-1, for the dev host and the largest member.
  • The sizing, the per-row cost against AURORA_RESERVE_S and the batching threshold are now documented in steampipe-quota-and-staleness.md and in the query comment.
  • Batching the shared writer is out of scope for this PR.
• MAJOR 46→49 sweep: done in scripts/CLAUDE.md and graph-read-contract.md. The generated AGENTS.md mirrors were in sync before this change; they get the identical count lines and fresh markers, and check_ai_context.py reports both in sync.
• Disputed column name: attached_instance_id is correct. It was checked against table_aws_ec2_network_interface.go at plugin tag v0.142.0, together with every other new column (including custom_endpoints, members, db_subnet_group, create_time, and proxy tags/updated_date). Provenance comments were added.
• Minors: fixed two: rds_proxy now selects name, and the ENI comment is scoped to private IPv4.

Superseded by #258, which now carries both the collector and the web change: reviewed separately, each half was judged incomplete without the other.